docs: add deployed-contracts.md registry and link it in documentation…#1047
docs: add deployed-contracts.md registry and link it in documentation…#1047K1NGD4VID wants to merge 9 commits into
Conversation
…Url type and enhance webhook delivery logging
…ng tests expect undefined instead of null
…e proper type omission in notifications
…ns; skip DB lookups for synthetic users
…ectLoanTx for consistency
ogazboiz
left a comment
ogazboiz
left a comment
There was a problem hiding this comment.
hey, the docs/deployed-contracts.md addition itself looks fine and closes the doc gap, but this PR has serious scope problems that block merging:
-
Security smell in production middleware.
backend/src/middleware/jwtAuth.tsadds anif (process.env.NODE_ENV === "test")block that accepts hardcodedtest-token/test-admin-tokenstrings as authenticated users.backend/src/controllers/loanController.tsdoes the same fortest-*user prefixes. NODE_ENV branches inside production code paths are a footgun — a misconfigured deploy or a downstream consumer setting NODE_ENV=test reopens the backdoor. Test-only behavior should live in test fixtures/mocks, not in shipped middleware. -
Scope creep. The PR title is "docs: add deployed-contracts.md" but it touches 10 files including app.ts (adds backwards-compat root mounts), loanController, jwtAuth, loanAccess, notificationService, webhookService, and frontend/utils/amount.ts. Those changes need their own scoped PRs.
To unblock: please split this into:
- one PR with only
docs/deployed-contracts.md+ the README/CONTRIBUTING links pointing to it (closes #958) - separate PRs (if the changes are actually needed) for the app mounts, the controller refactors, and the amount.ts update — none with NODE_ENV backdoors
happy to review the doc-only PR fast once it's split out.
|
fixed, please review |
2 participants
closes #958